Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib,src: make constants not inherit from Object #10458

Conversation

thefourtheye
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

lib src test

Description of change

Make sure constants object and all the nested objects don't inherit
from Object.prototype but from null.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. process Issues and PRs related to the process subsystem. labels Dec 26, 2016
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 26, 2016
lib/fs.js Outdated
@@ -1004,7 +1004,7 @@ fs.fchmodSync = function(fd, mode) {
return binding.fchmod(fd, modeNum(mode));
};

if (constants.hasOwnProperty('O_SYMLINK')) {
if ('O_SYMLINK' in constants) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these have a small performance impact, maybe Object.prototype.hasOwnProperty.call is better for all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As constants inherit from null, will the performance implication still be applicable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance of the in operator has recently been optimized in V8 (in 5.2).

Copy link
Contributor

@mscdex mscdex Dec 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@targos It's still quite slow though in master (V8 5.4). I recently swapped usage of in with a simpler undefined check and saw a ~50% improvement.

@thefourtheye I would just do an undefined check (constants.O_SYMLINK !== undefined) instead of in. It should still work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I changed them to !== undefined.

@targos
Copy link
Member

targos commented Dec 26, 2016

What do we gain from this change?

@thefourtheye
Copy link
Contributor Author

@targos Lookup should be comparatively faster and problems because of accidental accessing of inherited properties will be avoided, like in #10423

@thefourtheye thefourtheye force-pushed the make-constants-inherit-from-null branch from 11a0b88 to 8638f31 Compare December 26, 2016 18:58
Object.keys(constants.os).sort(), ['UV_UDP_REUSEADDR', 'errno', 'signals']
);

// make sure all the constants objects don't inherit from Object.prototype
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you capitalize "make" please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

);

// make sure all the constants objects don't inherit from Object.prototype
const INHERITED_PROPERTIES = Object.getOwnPropertyNames(Object.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you name this with camelCase please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@thefourtheye thefourtheye force-pushed the make-constants-inherit-from-null branch from 8638f31 to aaf7165 Compare December 26, 2016 19:18
@mscdex
Copy link
Contributor

mscdex commented Dec 27, 2016

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for a major (if it works, ci)

@mscdex
Copy link
Contributor

mscdex commented Jan 3, 2017

LGTM

@jasnell
Copy link
Member

jasnell commented Jan 3, 2017

I'm wondering if a similar semver-major change should not be made to the docs-only deprecated require('constants')

@thefourtheye
Copy link
Contributor Author

@jasnell Do you mean the object returned by require('constants') should also inherit from null?

@thefourtheye thefourtheye force-pushed the make-constants-inherit-from-null branch from aaf7165 to 002ce78 Compare January 3, 2017 18:18
Make sure `constants` object and all the nested objects don't inherit
from `Object.prototype` but from `null`.
@thefourtheye thefourtheye force-pushed the make-constants-inherit-from-null branch from 002ce78 to 3a75d7c Compare January 3, 2017 18:19
@jasnell
Copy link
Member

jasnell commented Jan 3, 2017

@thefourtheye ... yes, that's what I'm thinking. We could choose to leave it untouched but for consistency it may make sense

@thefourtheye
Copy link
Contributor Author

@jasnell I just tried that and it affects graceful-fs, https://github.com/isaacs/node-graceful-fs/blob/v4.1.11/polyfills.js#L31 :'(

@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

Boo. Ok, I'd leave it as is then.

@jasnell
Copy link
Member

jasnell commented Jan 10, 2017

@thefourtheye
Copy link
Contributor Author

cc @nodejs/ctc as this is a major change.

@thefourtheye
Copy link
Contributor Author

Yet another CI Run to BUMP https://ci.nodejs.org/job/node-test-pull-request/6372/

@jasnell
Copy link
Member

jasnell commented Feb 13, 2017

@thefourtheye ... this should be good to go!

@thefourtheye
Copy link
Contributor Author

@jasnell Thanks :-) I would be comfortable if I get a green CITGM as well, as this is a major change. I'll try to spend sometime this week to fix it.

@thefourtheye
Copy link
Contributor Author

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

CITGM and CI both were good. Landing.

jasnell pushed a commit that referenced this pull request Mar 22, 2017
Make sure `constants` object and all the nested objects don't inherit
from `Object.prototype` but from `null`.

PR-URL: #10458
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Landed in caf9ae7

@jasnell jasnell closed this Mar 22, 2017
@thefourtheye thefourtheye deleted the make-constants-inherit-from-null branch April 2, 2017 07:17
@jasnell jasnell mentioned this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants